Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(dom-size): display metric values as integers #14479

Merged
merged 5 commits into from
Nov 1, 2022
Merged

core(dom-size): display metric values as integers #14479

merged 5 commits into from
Nov 1, 2022

Conversation

shogohida
Copy link
Contributor

Summary

Fixes #14422

Related Issues/PRs

I think this issue was caused by the changes in this PR

@google-cla
Copy link

google-cla bot commented Oct 31, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@shogohida shogohida marked this pull request as ready for review October 31, 2022 14:36
@shogohida shogohida requested a review from a team as a code owner October 31, 2022 14:36
@shogohida shogohida requested review from brendankenny and removed request for a team October 31, 2022 14:36
@shogohida shogohida mentioned this pull request Oct 31, 2022
Closed
@adamraine
Copy link
Member

This won't actually change the value because the numbers are already integers. The problem is that the Lighthouse report displays the integers with unnecessary granularity.

To fix this, we need to change the details items from

{
  statistic: str_(UIStrings.statisticDOMElements),
  value: stats.totalBodyElements,
}

to

{
  statistic: str_(UIStrings.statisticDOMElements),
  value: {
    type: 'numeric',
    granularity: 1,
    value: stats.totalBodyElements,
  },
}

@shogohida
Copy link
Contributor Author

@adamraine
Thanks for your comment! I will push the changes when I finish

@shogohida shogohida marked this pull request as draft November 1, 2022 04:23
@connorjclark
Copy link
Collaborator

connorjclark commented Nov 1, 2022 •
edited
Loading

Please confirm your changes locally via yarn start https://www.example.com --view.

@shogohida
Copy link
Contributor Author

@connorjclark @adamraine
I added settings of value in this commit as Adam indicated.

In addition, I checked there is no .0 with the command Connor taught me.
スクリーンショット 2022-11-01 22 56 37

@shogohida shogohida marked this pull request as ready for review November 1, 2022 14:09
@adamraine
Copy link
Member

adamraine commented Nov 1, 2022 •
edited
Loading

Some of our tests are failing, to fix this you should

  1. run yarn update:sample-json and commit the changes
  2. Update the unit test expectations, verify with yarn unit-core

lighthouse/core/test/audits/dobetterweb/dom-size-test.js

Lines 24 to 32 in 60c2fa2

it('calculates score hitting mid distribution', () => {
const auditResult = DOMSize.audit(artifact, context);
assert.equal(auditResult.score, 0.43);
assert.equal(auditResult.numericValue, numElements);
expect(auditResult.displayValue).toBeDisplayString('1,500 elements');
assert.equal(auditResult.details.items[0].value, numElements);
assert.equal(auditResult.details.items[1].value, 1);
assert.equal(auditResult.details.items[2].value, 2);
});

  1. Update the smoke test expectations, verify with yarn smoke dbw

lighthouse/cli/test/smokehouse/test-definitions/dobetterweb.js

Lines 403 to 417 in 60c2fa2

'dom-size': {
score: 1,
numericValue: 153,
details: {
items: [
{statistic: 'Total DOM Elements', value: 153},
{statistic: 'Maximum DOM Depth', value: 4},
{
statistic: 'Maximum Child Elements',
value: 100,
node: {snippet: '<div id="shadow-root-container">'},
},
],
},
},

@shogohida
Copy link
Contributor Author

@adamraine
Thanks for your comment! Your support always helps me.

I made changes in the commits below.
1aa78ac
e87d529
aabd3db

A test case of difference at total-blocking-time audit.numericValue fails but it seems there is no relation between that failure and this PR.

https://github.com/GoogleChrome/lighthouse/actions/runs/3371728139/jobs/5594423467

adamraine
adamraine approved these changes Nov 1, 2022
View reviewed changes
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test case of difference at total-blocking-time audit.numericValue fails but it seems there is no relation between that failure and this PR.

You are correct, this is a common flaky failure #14271. This PR looks good, thanks for the contribution!

@adamraine adamraine changed the title core(dom-size): make dom-size value integer core(dom-size): display metric values as integers Nov 1, 2022
@adamraine adamraine merged commit 38cce6a into GoogleChrome:main Nov 1, 2022
@shogohida shogohida deleted the make-dom-size-integer branch November 1, 2022 18:41
@shogohida
Copy link
Contributor Author

Thanks for your quick review! I hope to contribute more 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamraine adamraine adamraine approved these changes

@brendankenny brendankenny Awaiting requested review from brendankenny brendankenny is a code owner automatically assigned from GoogleChrome/lighthouse-hackers

Assignees

@brendankenny brendankenny

Labels
waiting4reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Too much precision on the DOM size audit
5 participants
@shogohida @adamraine @connorjclark @brendankenny @devtools-bot